Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tightly coupled market-commons with prediction markets. #900

Merged
merged 8 commits into from
Dec 13, 2022

Conversation

vivekvpandya
Copy link
Contributor

Fixes #805

@vivekvpandya vivekvpandya added the s:review-needed The pull request requires reviews label Dec 7, 2022
@vivekvpandya vivekvpandya self-assigned this Dec 7, 2022
Copy link
Member

@maltekliemann maltekliemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double-booking the implementation of MarketCommonsPalletApi into the implementation of the zrml_market_commons::Pallet::<T> seems strange. Is there a particular reason why you did that, instead of replacing every zrml_market_commons::Pallet::<T> with <zrml_market_commons::Pallet::<T> as MarketCommonsPalletApi>?

@vivekvpandya
Copy link
Contributor Author

Double-booking the implementation of MarketCommonsPalletApi into the implementation of the zrml_market_commons::Pallet::<T> seems strange. Is there a particular reason why you did that, instead of replacing every zrml_market_commons::Pallet::<T> with <zrml_market_commons::Pallet::<T> as MarketCommonsPalletApi>?

Frankly I did not get this idea when editing, I though for loose coupling we use MarketCommonsPalletApi and for tight coupling we directly use functions from pallet and don't need to import MarketCommonsPalletApi.

@Chralt98
Copy link
Member

Chralt98 commented Dec 9, 2022

Starting my review now..

@maltekliemann
Copy link
Member

Double-booking the implementation of MarketCommonsPalletApi into the implementation of the zrml_market_commons::Pallet::<T> seems strange. Is there a particular reason why you did that, instead of replacing every zrml_market_commons::Pallet::<T> with <zrml_market_commons::Pallet::<T> as MarketCommonsPalletApi>?

Frankly I did not get this idea when editing, I though for loose coupling we use MarketCommonsPalletApi and for tight coupling we directly use functions from pallet and don't need to import MarketCommonsPalletApi.

It's a matter of dependency. With loose coupling, you can only access the config parameter MarketCommons, which implements MarketCommonsPalletApi. This allows you to use any struct which implements that particular trait. With tight coupling, that config parameter is gone and we have to use the particular zrml_market_commons pallet, which happens to implement MarketCommonsPalletApi. In particular, the user has no choice in the matter. Before this change, they could have used their own "market manager". Now they have to use ours.

But since the MDMs are still loosely coupled, I think we should move MarketCommonsPalletApi into the primitives. Is there any argument against this?

@Chralt98
Copy link
Member

Chralt98 commented Dec 9, 2022

You like to have the tight coupling of zrml_swaps in another PR?

@vivekvpandya
Copy link
Contributor Author

You like to have the tight coupling of zrml_swaps in another PR?

yes if it is really required, as per discussion in one of the meeting we feel that swaps should not be tightly coupled with market-commons.

Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of the tightly couple.

zrml/market-commons/src/lib.rs Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
@vivekvpandya
Copy link
Contributor Author

But since the MDMs are still loosely coupled, I think we should move MarketCommonsPalletApi into the primitives. Is there any argument against this?

so what is the motivation here? Just that above trait is imported from primitives and not from market_commons? I mean only implementer of that trait is MarketCommons so I am fine with any approach.

@maltekliemann
Copy link
Member

But since the MDMs are still loosely coupled, I think we should move MarketCommonsPalletApi into the primitives. Is there any argument against this?

so what is the motivation here? Just that above trait is imported from primitives and not from market_commons? I mean only implementer of that trait is MarketCommons so I am fine with any approach.

Because the MDMs and swaps are still loosely coupled, people could potentially use them with some other struct that implements MarketCommonsPalletApi. If they wanted to do that, they would have to import zrml_market_commons to get the trait, which is an unnecessary import. Instead, they should be able to import the trait from the primitives.

zrml/prediction-markets/src/benchmarks.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@maltekliemann maltekliemann added the p:high High priority, prioritize the resolution of this issue label Dec 13, 2022
Copy link
Member

@maltekliemann maltekliemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@vivekvpandya vivekvpandya removed the s:review-needed The pull request requires reviews label Dec 13, 2022
@vivekvpandya vivekvpandya added the s:accepted This pull request is ready for merge label Dec 13, 2022
@vivekvpandya vivekvpandya merged commit 5d73988 into main Dec 13, 2022
@vivekvpandya vivekvpandya deleted the issue-805new branch December 13, 2022 17:06
@sea212 sea212 removed the p:high High priority, prioritize the resolution of this issue label Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:accepted This pull request is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PM][Swaps] Tightly couple market-commons pallet
4 participants